Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wallet revoke permissions #145

Merged
merged 12 commits into from
Nov 20, 2023
Merged

Wallet revoke permissions #145

merged 12 commits into from
Nov 20, 2023

Conversation

alaahd
Copy link
Contributor

@alaahd alaahd commented Jun 5, 2023

Description

This pull request adds the "wallet_revokePermissions" JSON-RPC method, which allows revoking permissions from the current domain. It provides the flexibility to revoke all permissions for a specified permission or revoke permissions for specific caveats like account addresses.

Changes Made

  • Add the "wallet_revokePermissions" method to the OpenRPC Doc.

@alaahd alaahd requested a review from a team as a code owner June 5, 2023 12:15
openrpc.json Outdated Show resolved Hide resolved
openrpc.json Outdated Show resolved Hide resolved
openrpc.json Outdated Show resolved Hide resolved
openrpc.json Outdated Show resolved Hide resolved
@vandan
Copy link
Contributor

vandan commented Sep 13, 2023

If this is going to change the public JSON-RPC API, then a MIP needs to be created for this change. Going through the process would further facilitate the review/acceptance/implementation cycle.

},
"value": {
"title": "CaveatValue",
"description": "Value of the caveat."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is technically any. any thoughts on other ways to represent this @BelfordZ

openrpc.json Outdated
Comment on lines 346 to 352
"result": {
"name": "UpdatedPermissionsAfterRevoke",
"description": "The list of the revoked permissions objects",
"schema": {
"$ref": "#/components/schemas/PermissionsList"
}
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this response is expected for a dApp that was already connected previously, will the wallet_revokePermissions RPC method will only be accessible to dApps who are connected (ie: its a restricted method)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could. It doesn't have to be though, it could just return an empty array if there are no existing permissions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw the update to the description. Makes sense now 👍

If we are allowing this method without being connected, we should ensure its resistant to timing attacks which can reveal if a specific snap or account exists on the device.

openrpc.json Outdated Show resolved Hide resolved
openrpc.json Outdated Show resolved Hide resolved
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@adonesky1 adonesky1 merged commit 8e50a74 into main Nov 20, 2023
5 checks passed
@adonesky1 adonesky1 deleted the wallet-revoke-permissions branch November 20, 2023 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants